Skip to content

Conversation

@marko-bekhta
Copy link
Contributor

@marko-bekhta marko-bekhta commented Mar 27, 2025

Check list:

Your pull request:

  • targets the development branch
  • uses the 999-SNAPSHOT version of Quarkus
  • has tests (mvn clean test)
  • works in native (mvn clean package -Pnative)
  • has integration/native tests (mvn clean verify -Pnative)
  • makes sure the associated guide must not be updated
  • links the guide update pull request (if needed)
  • updates or creates the README.md file (with build and run instructions)
  • for new quickstart, is located in the directory component-quickstart
  • for new quickstart, is added to the root pom.xml and README.md

@marko-bekhta marko-bekhta force-pushed the feat/add-jakarta-data-quick-start branch from 35c4cea to a890883 Compare April 18, 2025 10:22
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM, but it's still a draft? Is there still something missing?

@marko-bekhta
Copy link
Contributor Author

hey 👋🏻

Is there still something missing?

  • I wanted to wait for the PR Luca opened to get merged first to see if there'd be any other updates there, but I think it's already in.
  • I noticed some "issues" with transactions/cdi contexts and repositories so wanted to check with Gavin to see what he thinks about it HHH-19109 fix use of @TransactionScoped by repository hibernate/hibernate-orm#9855 (comment). but now as it got changed (hibernate/hibernate-orm@e0c81b9) I'm not sure whether to wait for that update to be released or leave things as they currently are in this PR.
  • Guillaume had that suggestion for the readme, so I was just waiting to see what Guillaume and you decide that we want to do here 😃

Other than that ... 🟢 😃

@yrodiere
Copy link
Member

hey 👋🏻

Is there still something missing?

* I wanted to wait for the PR Luca opened to get merged first to see if there'd be any other updates there, but I think it's already in.

It's in.

* I noticed some "issues" with transactions/cdi contexts and repositories so wanted to check with Gavin to see what he thinks about it [HHH-19109 fix use of @TransactionScoped by repository hibernate/hibernate-orm#9855 (comment)](https://github.com/hibernate/hibernate-orm/pull/9855#discussion_r2050448996). but now as it got changed ([hibernate/hibernate-orm@e0c81b9](https://github.com/hibernate/hibernate-orm/commit/e0c81b97ecfa8ad54fda498f0196fa545be76c51)) I'm not sure whether to wait for that update to be released or leave things as they currently are in this PR.

The best practice is to use @Transactional everywhere. You can put it on the REST Resource class if that's easier. I wouldn't worry about other (@Transactional-less) use cases for this PR.

* Guillaume had that suggestion for the readme, so I was just waiting to see what Guillaume and you decide that we want to do here 😃

He suggested to remove it, IIRC? Or at least replace it with just a title + link to docs. Fine by me, but should be a separate PR that would need to address all Hibernate quickstarts. So it's fine to merge your PR as is.

So... merging :)

@yrodiere yrodiere marked this pull request as ready for review April 24, 2025 08:51
@yrodiere yrodiere merged commit b830687 into quarkusio:development Apr 24, 2025
2 checks passed
@yrodiere
Copy link
Member

@marko-bekhta Did you link to this quickstart from the Quarkus docs? You probably should, otherwise nobody will notice it :)

@marko-bekhta
Copy link
Contributor Author

You probably should

thanks for the suggestion 😃 will send a PR later 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants